Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE-15552 Add findUnconsumedStatesByExactType paging API #1292

Merged

Conversation

lankydan
Copy link
Collaborator

@lankydan lankydan commented Oct 11, 2023

Add findUnconsumedStatesByExactType to UtxoLedgerService , allowing developers to retrieve states by type from the vault in pages. This dissuades developers from retrieving every state from the vault in one go and returning them to a flow.

@lankydan lankydan requested a review from a team October 11, 2023 11:56
@lankydan lankydan self-assigned this Oct 11, 2023
@lankydan lankydan requested a review from a team as a code owner October 11, 2023 11:56
@corda-jenkins-ci02
Copy link
Contributor

Non-blocking downstream job failed for corda-e2e-test

https://ci02.dev.r3.com/job/Corda5/job/corda-api-compatibility/job/PR-1292/1/ has failed for PR 1292 build 1

Please investigate if your changes may have broken compilation on https://github.com/corda/corda-e2e-tests

Copy link
Contributor

@relyafi relyafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR needs a bit more background adding to the description, and a link to the runtime-os PR.

<T extends ContractState> ResultSet<StateAndRef<T>> findUnconsumedStatesByExactType(
@NotNull Class<T> type,
@NotNull Integer limit,
@NotNull Instant createdTimestampLimit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, this is a change beyond what is claimed by this PR. If we want to add this, then the PR title / description should reflect this as well.

Do we actually have a use-case for this? If the intent is just to limit it to what exists at the start of the query, then this could just be done internally. Or we could actually just allow it to get the latest results until it runs out of results to return, and not set any limit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps its functionality in line with the vault named query functionality. I think it makes sense to restrict it in the same way we do there, especially since the underlying mechanism is the same between them. We can do it internally if we wanted to, but either way it needs setting whether publicly or internally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then the question is whether we want to force users to have to provide it, or we provide another overload / a default argument that just sets it to now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the fact users have to provide a timestamp on this API - there should at least be an override that defaults to now().

@lankydan
Copy link
Collaborator Author

A question in regards to this PR.

Should we remove the findUnconsumedStatesByExactType that does not support paging? The API was added in 5.1 (the current release) so it hasn't been released yet. Instinctively I think that is the right decision, especially since I considered deprecating it.

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Oct 11, 2023

Jenkins build for PR 1292 build 7

Build Successful:
Jar artifact version produced by this PR: 5.1.0.36-alpha-1697207552373

@relyafi
Copy link
Contributor

relyafi commented Oct 11, 2023

A question in regards to this PR.

Should we remove the findUnconsumedStatesByExactType that does not support paging? The API was added in 5.1 (the current release) so it hasn't been released yet. Instinctively I think that is the right decision, especially since I considered deprecating it.

That sounds reasonable if it's not been released yet. We can also ask the question whether we should support paging on findUnconsumedStatesByType, but I think that is deprecated now anyway, in which case there is no point?

@lankydan
Copy link
Collaborator Author

A question in regards to this PR.
Should we remove the findUnconsumedStatesByExactType that does not support paging? The API was added in 5.1 (the current release) so it hasn't been released yet. Instinctively I think that is the right decision, especially since I considered deprecating it.

That sounds reasonable if it's not been released yet. We can also ask the question whether we should support paging on findUnconsumedStatesByType, but I think that is deprecated now anyway, in which case there is no point?

Agreed, I think there is no point. In fact, I suggest we @Deprecate it if we haven't already.

vlajos
vlajos previously approved these changes Oct 12, 2023
vlajos
vlajos previously approved these changes Oct 13, 2023
@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from d76e927 to 1326493 Compare October 13, 2023 10:39
vlajos
vlajos previously approved these changes Oct 13, 2023
@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from 4c83854 to 183ea31 Compare October 13, 2023 13:21
@blsemo blsemo added the 5.1 label Oct 13, 2023
vlajos
vlajos previously approved these changes Oct 13, 2023
@lankydan lankydan force-pushed the dan/CORE-15552-unconsumed-states-by-type-paging branch from 183ea31 to 1815602 Compare October 13, 2023 14:32
@lankydan lankydan merged commit 40eb1aa into release/os/5.1 Oct 13, 2023
@lankydan lankydan deleted the dan/CORE-15552-unconsumed-states-by-type-paging branch October 13, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants